Skip to content

ARROW-5196 [C++] Uniform usage of Google cpu_features library accross the codebase#4201

Closed
aregm wants to merge 8 commits into
apache:masterfrom
aregm:ARROW-5196
Closed

ARROW-5196 [C++] Uniform usage of Google cpu_features library accross the codebase#4201
aregm wants to merge 8 commits into
apache:masterfrom
aregm:ARROW-5196

Conversation

@aregm

@aregm aregm commented Apr 25, 2019

Copy link
Copy Markdown

Resolves the ARROW-5196, Draft pull request to check on various platforms.

@wesm

wesm commented Apr 25, 2019

Copy link
Copy Markdown
Member

Hm, I erroneously got the idea that the cpu_features library was more minimalistic than it actually is, I apologize for proposing to vendor it. @pitrou do you think we should add this to conda-forge and the thirdparty toolchain?

@pitrou

pitrou commented Apr 25, 2019

Copy link
Copy Markdown
Member

Is there a more lightweight alternative to this library? I'm not even sure we're using any of this currently, do we?

@wesm

wesm commented Apr 25, 2019

Copy link
Copy Markdown
Member

At one point we were querying supported SIMD instructions; I think it's important to determine at least

  • CPU cache sizes (since algorithms can behave much differently when calibrated based on these)
  • whether AVX2 and AVX512 are available

@pitrou

pitrou commented Apr 25, 2019

Copy link
Copy Markdown
Member

Ok, it seems that cpu_features is our most reliable option.

We may want to make a conda-forge package ultimately. Though first we can simply add it to our thirdparty libs and build it on the fly (it's actually very fast to build by default).

@pitrou

pitrou commented Apr 25, 2019

Copy link
Copy Markdown
Member

Side note: cpu_features also provides an executable which can output CPU info in JSON format @fsaintjacques :

$ list_cpu_features 
arch            : x86
brand           : AMD Ryzen 7 1700 Eight-Core Processor          
family          :  23 (0x17)
model           :   1 (0x01)
stepping        :   1 (0x01)
uarch           : AMD_ZEN
flags           : aes,avx,avx2,bmi1,bmi2,cx16,f16c,fma3,movbe,popcnt,rdrnd,sha,sse4_1,sse4_2,ssse3
$ list_cpu_features --json
{"arch":"x86","brand":"AMD Ryzen 7 1700 Eight-Core Processor          ","family":23,"model":1,"stepping":1,"uarch":"AMD_ZEN","flags":["aes","avx","avx2","bmi1","bmi2","cx16","f16c","fma3","movbe","popcnt","rdrnd","sha","sse4_1","sse4_2","ssse3"]}

@aregm

aregm commented Apr 25, 2019

Copy link
Copy Markdown
Author

@pitrou I'll create conda package and add to conda-forge, and redo the PR accordingly. BTW AppVeyor is failing with an error: "CMake Error at C:/Program Files (x86)/CMake/share/cmake-3.13/Modules/FindPackageHandleStandardArgs.cmake:137 (message):
Could NOT find DoubleConversion (missing: DoubleConversion_LIB
DoubleConversion_INCLUDE_DIR)"
But I cannot reproduce i neither in our build system, nor in local build. Have you encountered this before?

@pitrou

pitrou commented Apr 25, 2019

Copy link
Copy Markdown
Member

@aregm You shouldn't lose your breath over AppVeyor for now ;-) The priority is to revamp the PR using the discussed approach (i.e. build cpu_features using our usual "thirdparty toolchain" build chain, rather than vendor it).

@aregm

aregm commented Apr 25, 2019

Copy link
Copy Markdown
Author

@pitrou, I am just curious what is the cause, but do not plan to spend time on it :)
I plan to create conda package and not another folder thirdparty. I think it is more clean solution. Python guys will help me to publish it so, everybody will benefit.

@pitrou

pitrou commented Apr 25, 2019

Copy link
Copy Markdown
Member

We still need to allow building it from cmake_modules/ThirdpartyToolchain.cmake. conda is not a requirement for building Arrow.

@aregm

aregm commented Apr 30, 2019

Copy link
Copy Markdown
Author

@pitrou @wesm I have added cpu_features to the conda-forge, but as it was suggested to add to thirdparty - do you have any fast "best-known method" or doc how to add external dependency to the Thirdparty with all the targets and defines correctly propagated through cmakefiles - I tried to replicate double-conversion way, but still includes and libs do not found, and I do not want to spend time on deciphering all the nested cmake calls in --trace if there is a faster way and I do not have to. Thanks!

@pitrou

pitrou commented Apr 30, 2019

Copy link
Copy Markdown
Member

Unfortunately each external dependency requires its own treatment (the sad fact is that there's no standard build system or procedure in C++), so there's no "easy" way to do it. Usually I would first try to build the dependency by hand, observe the exact options required and where the various build products are installed (include files, library files), and start from there to try replicating it from CMake.

Of course, you can still steal some inspiration for the CMake boilerplate from the existing thirdparty declarations.

More generally you need to:

  • add new entries in thirdparty/versions.txt
  • add a new entry in ARROW_THIRDPARTY_DEPENDENCIES
  • add a new entry in the build_dependency macro
  • add a URL definition snippet (for downloading) as for other thirdparty libs
  • define the build_XXX macro for your XXX dependency
  • add some code that detects the third-party library and/or calls into build_dependency

@emkornfield

Copy link
Copy Markdown
Contributor

@aregm what is the status of this PR?

@aregm

aregm commented Jun 14, 2019

Copy link
Copy Markdown
Author

@emkornfield The status of feature is pending - I need to add cache info parsing to cpu_features as it occurs they are not parsed there and then pull it in. If you are ok, to go without cache info for a while, until the next release, then I can do it as is.

@kszucs kszucs force-pushed the master branch 2 times, most recently from ed180da to 85fe336 Compare July 22, 2019 19:29
@aregm aregm closed this Aug 9, 2019
@aregm aregm reopened this Aug 21, 2019
@aregm aregm marked this pull request as ready for review August 21, 2019 02:54
@bkietz

bkietz commented Aug 22, 2019

Copy link
Copy Markdown
Member

@aregm looks like you have merge conflicts, could you please rebase this on master?

@emkornfield

Copy link
Copy Markdown
Contributor

@aregm I'm going to close this PR for now, please reopen if you have time to rebase, and address any additional feedback. If I understand correctly I think this got blocked on appropriate integration with the build system?

@aregm

aregm commented Oct 16, 2019

Copy link
Copy Markdown
Author

@emkornfield Integration is done in PR, but is old and needs to be rebased on top of the latest master. Need to do that as soon as I get some free cycles. If you need this, I'll do this week.

@emkornfield

Copy link
Copy Markdown
Contributor

@aregm, I don't think its urgent, I'm just trying to clear out PRs that appear to have stagnated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants